HDDS-15417. Handle mixed datanode versions on the replication path#10570
HDDS-15417. Handle mixed datanode versions on the replication path#10570dombizita wants to merge 10 commits into
Conversation
github-actions
Bot
added
the
zdu
…mment to getLowestCommonApparentVersion.
errose28
left a comment
errose28
left a comment
There was a problem hiding this comment.
Thanks for adding this @dombizita. Left some comments, mostly around a few high level ideas:
- IMO we can assume that the pull replicator will be removed from master by the time this lands, so we should not reference it here.
- SCM should decide the version to use in the command. The command itself should be a vessel for whatever version is provided and datanodes should use the version they are instructed without needing to know why SCM has made that decision.
- We should use the typed
HDDSVersioninstead of a serialized int up until it needs to be converted to a proto. - We should fail fast on the replication path instead of trying to absorb errors for cases like missing node information or unexpected versions which may lead to incorrect behavior.
|
In the last commit I moved |
errose28
left a comment
errose28
left a comment
There was a problem hiding this comment.
Thanks for the updates. The PR description needs to be updated since the min computation is now on SCM.
| throw new NodeNotFoundException(dn.getID()); | ||
| } | ||
| HDDSVersion version = info.getApparentHddsVersion(); | ||
| if (version.serialize() < lowest.serialize()) { |
There was a problem hiding this comment.
Ideally nothing outside the version class would be comparing serialized values. Maybe we add a static ComponentVersion.min(v1, v2) method to handle this.
There was a problem hiding this comment.
Added ComponentVersion.min(ComponentVersion...) method.
| } | ||
|
|
||
| /** | ||
| * Returns the lowest apparent (finalized) version among the given datanodes, |
There was a problem hiding this comment.
| * Returns the lowest apparent (finalized) version among the given datanodes, | |
| * Returns the lowest apparent version among the given datanodes, |
| "apparent version newer than SCM recognizes. SCM must be upgraded " + | ||
| "before datanodes."); | ||
| } | ||
| return HDDSVersion.deserialize(apparentVersion.serialize()); |
There was a problem hiding this comment.
The version returned by the datanodes may be an HDDSVersion or an HDDSLayoutFeature. This in-place serde is just erasing the type. Really this whole method should be deleted. We should add ComponentVersion.min(ComponentVersion...) and pass it getLastKnownApparentVersion for all Datanodes we are considering. If you check the constructor of this class you'll see that we already assume the version info is always present, otherwise we'd get an NPE. That part could possibly be improved, even though DNs are expected to send version info on every heartbeat.
2 participants
What changes were proposed in this pull request?
Datanodes act in a client server relationship between themselves for replication. In general the replication process is quite simple - commands are sent to a datanode hosting a container replica (the replication client), and it is told to push/upload the data to another node (the replication server). Incompatible changes in this area can be handled in a similar way to write requests.
SCM knows the apparent version of all Datanodes since their last heartbeat. The last known apparent version of the target/server Datanode can be included in the replicate command sent by SCM to the source/client Datanode. It is always the job of the newer component to handle compatibility, whether it involves software or apparent version. If the source/client Datanode is newer in software or apparent version, it must "downgrade" its request to the lowest common apparent version supported by the server indicated by the SCM command. A newer target/server Datanode will not make incompatible changes to existing APIs, so old client Datanodes will continue to work.
Changes in this patch:
Used Claude Opus 4.6.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15417
How was this patch tested?
Green CI on my fork: https://github.com/dombizita/ozone/actions/runs/27760621062